-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Math to get min scale and max scale in Viewbox.ComputeScaleFactor #3463
Conversation
What is the performance impact of this change? |
@miloush Due to the method call, the performance is a little worse than before, but the code is clearer. I don't know should I pr this. If we need to consider the performance, there are lots of |
It's unfortunate but we don't have automated perf tests running on WPF NET5. This is critical path code, so I wouldn't spend time on this PR at the moment. |
@h82258652 That's not even true in general, the method call might get optimized away. It depends on the compiler, jitter, platform, where the values come from, their data types etc. Being able to state either "under conditions X the performance improvement is Y and every WPF app hits this line 100 times at start-up" or "we get more readable/consistent code for the performance/size cost of X and the code is rarely ever hit" helps people to make an informed decision on whether the change is worth the effort and risk. |
FYI with dotnet/runtime#33851 |
I performed a perf test and it shows that using Math.Max/Math.Min is 4-5 times slower then the default. Here are the results: If we take this PR now, the only output we get is a clean code at the cost of reduced performance. As such, we are closing this PR until the time there is a change in underlying functions which has atleast same or comparable performance. |
Not mentioning the fact that it changes behaviour. If |
No description provided.